feat(lint/noEmptyBlockStatements): add rule #521
Conversation
github-actions
bot
added
A-Project
andrewangelle
force-pushed
the
lint/no-empty-block-statements
branch
from
b26af0b
to
59a55b5
Compare
andrewangelle
force-pushed
the
lint/no-empty-block-statements
branch
from
59a55b5
to
deee494
Compare
crates/biome_js_analyze/src/analyzers/nursery/no_empty_block_statements.rs
Outdated
Show resolved
Hide resolved
| /// ```js,expect_diagnostic | ||
| /// function foo () {} | ||
| /// | ||
| /// const foo = () => {} | ||
| /// | ||
| /// function fooWithNestedEmptyBlock() { | ||
| /// let a = 1; | ||
| /// function shouldFail(){} | ||
| /// return a | ||
| /// } | ||
| /// | ||
| /// const fooWithNestedEmptyBlock = () => { | ||
| /// let a = 1; | ||
| /// const shouldFail = () => {} | ||
| /// return a | ||
| /// } | ||
| /// let someVar; | ||
| /// if (someVar) { | ||
| /// } | ||
| /// | ||
| /// while (someVar) { | ||
| /// } | ||
| /// | ||
| /// switch(someVar) { | ||
| /// } | ||
| /// try { | ||
| /// doSomething(); | ||
| /// } catch(ex) { | ||
| /// | ||
| /// } finally { | ||
| /// | ||
| /// } | ||
| /// | ||
| // class Foo { | ||
| // static {} | ||
| // } | ||
| /// ``` |
There was a problem hiding this comment.
Every example that emits a diagnostic should be in its own code snippet.
By the way, we don't need to be exhaustive. You could choose a few examples that demonstrates the scope of the rule.
| /// | ||
| /// ## Valid | ||
| /// | ||
| /// ```js |
There was a problem hiding this comment.
Same here: we should keep only a few examples.
| if is_empty && !has_comments { | ||
| return Some(text_range); | ||
| } | ||
|
|
||
| None |
There was a problem hiding this comment.
| if is_empty && !has_comments { | |
| return Some(text_range); | |
| } | |
| None | |
| (is_empty && !has_comments).then_some(()) |
|
|
||
| impl Rule for NoEmptyBlockStatements { | ||
| type Query = Ast<Query>; | ||
| type State = TextRange; |
There was a problem hiding this comment.
Because we use the range of the queried node, I think we can use () as state and directly call query.range() in diagnostics (see proposed changes).
| type State = TextRange; | |
| type State = (); |
| Some( | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| state, |
There was a problem hiding this comment.
query.range() internally calls query.syntax().text_trimmed_range().
| state, | |
| query.range(), |
| rule_category!(), | ||
| state, | ||
| markup! { | ||
| "No empty blocks allowed." |
There was a problem hiding this comment.
| "No empty blocks allowed." | |
| "Unexpected empty block." |
| }, | ||
| ) | ||
| .note(markup! { | ||
| "Empty static blocks and block statements, while not technically errors, usually occur due to refactoring that wasn’t completed. They can cause confusion when reading code." |
There was a problem hiding this comment.
We recently updated the analyzer's contributing guide to help to write a diagnostic. We should suggest a way to fix the issue. Also, I propose some simplification:
| "Empty static blocks and block statements, while not technically errors, usually occur due to refactoring that wasn’t completed. They can cause confusion when reading code." | |
| "Empty blocks are usually the result of an incomplete refactoring. Remove the empty block or add a comment inside it if it is intentional." |
| static {} | ||
| } | ||
|
|
||
| for(let i; i>0; i++){} |
There was a problem hiding this comment.
Last thing before merging: do you think it makes sense adding an exception for all loop statements?
There was a problem hiding this comment.
Can you think of a concrete and/or common example where a developer wants to execute a loop without doing anything in the iterations?
My initial thought is that would be pretty uncommon, and they could just inline suppress the rule or comment in the block if they really wanted it there.
There was a problem hiding this comment.
Just my two cents. I'm not opposed to adding an exception if you think it's appropriate.
There was a problem hiding this comment.
Let's ship the rule as is. We see later if users complain about the default behavior.
Summary
Implements no empty and no empty static block under 1 new rule for
noEmptyBlockStatementsAddresses issue: closes #39
Test Plan
Added a variety of snapshot tests included in the PR